-
Notifications
You must be signed in to change notification settings - Fork 15
Bump RustCrypto dependency crates to Release Candidate version #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@stevefan1999-personal can you take a look at this? I know it duplicates some of the work you were trying to do in #87, but I do like that this is just a self-contained dependency upgrade PR |
|
@tarcieri I'm out on a trip rn and I will be back on 2nd Jan, but I will try to take a look |
|
Okay I'm back from Japan now, seems like I can start working on it too |
|
@tarcieri @stevefan1999-personal gentle ping :) Also check out this suggestion. #103 (comment) |
|
I will say as a general comment I prefer this PR over #103 simply by virtue of being smaller and more reviewable |
|
@tarcieri @stevefan1999-personal |
|
I can do a more detailed review soon |
Please do so when you have time. :) There are some points that concern me about the content addressed in this PR. When updating dependencies, I noticed that However, the release currently includes code to simulate this. I believe that cryptography implementation should be left to the RustCrypto crates, which are implemented by experts, and that If this proves to be the case, I'd be happy to remove the separate implementation for sec1 and change it to return an error indicating that it's unsupported. |
|
Ed25519 defines its own point encoding which is not specified in SEC1 (it's a 255-bit compressed Edwards-y coordinate, along with 1-bit representing the sign/oddness, for a total of 256-bits/32-bytes). The original Ed25519 paper wasn't even published until 2 years after the latest published revision to SEC1 (V2 in 2009, vs 2011 for the Ed25519 paper). I'm not sure where you got |
This was certainly the case before this PR and this statement was valid. rustls-rustcrypto/src/sign/eddsa.rs Line 27 in 992778d
This PR would be even simpler if no replacement implementation was needed. |
|
That must've been going through the legacy blanket impl in the old version of the https://docs.rs/sec1/latest/sec1/trait.DecodeEcPrivateKey.html#implementors It's completely wrong though and has been redesigned/removed in the latest prereleases. Can you post an example of the ASN.1 DER you're trying to decode? |
I don't have a specific use case. I added a compatibility implementation based on what was supported up until now, but it turned out to be wrong. |
|
I'm guessing whatever it was, it likely doesn't work, but if you can somehow write a test case and print out the associated ASN.1 DER, I can help figure out how it should actually be implemented |
Thank you. However, as I commented earlier, I don't have a specific use case at the moment, so I think it's something we can think about when we need it. |
I'd be honored if I could be of help. :)
That certainly makes sense. However, having the "=" makes it a bit inconvenient for me to run cargo update on my laptop and try out the latest assets, so I haven't done that until now.
Thank you for your valuable suggestion! I would like to avoid having my own implementation if possible, so I will try this as well. I'd love to try it out right away, but I'm traveling for work for the next few days. |
|
@ColinFinck @tarcieri For these two targets, getrandom currently does not have a backend available and a custom backend is required.
Personally, I don't think it should support anything that getrandom doesn't. |
|
We generally tend to support using either the
Sidebar: I'm in the middle of cutting another round of crate releases for the |
Well the problem of rand is that they had quite a lot of ABI breakage lately, which from a semver level makes sense but not on DX. getrandom is quite stable but you need to have a global flag for custom backend, plus the problem of needing a C function export for getting random data, while rand (or rand_core) defines a trait that you implement the way to get source from. One is global system entropy, and the other one is pluggable and modular. If I want to get things done, getrandom that is, but if I want a more rigious approach I think we need to wait for rand to be stable |
|
@stevefan1999-personal Thank you for your opinion on I don't have the knowledge to properly assess the situation Therefore, we would like to switch to using On the other hand, EDIT: I am currently on a business trip and will be back on Friday night. I apologize for keeping you waiting... |
|
Yeah, I tend not to use
|
|
@tarcieri I have fixed it, so please leave a review when you have time. :) |
|
@tarcieri All crates have been replaced with ones based on |
| target: | ||
| - armv7a-none-eabi | ||
| - thumbv7em-none-eabi | ||
| # - armv7a-none-eabi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No luck with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the change to relying on getrandom for random number generation, such bare-metal targets are no longer supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems unfortunate, though I guess it might work if they plug in a custom backend, there just won't be CI.
Maybe there could at least be a dummy project with a fake backend that makes sure it builds?
src/sign/ecdsa.rs
Outdated
| $signing_key::from_sec1_der(sec1.secret_sec1_der()).map_err(|e| format!("failed to decrypt private key: {e}")) | ||
| }, | ||
| PrivateKeyDer::Pkcs1(_) => Err(format!("ECDSA does not support PKCS#1 key")), | ||
| PrivateKeyDer::Sec1(_) => Err(format!("ECDSA does not support SEC1 key")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ECDSA should support SEC1 keys. It's Curve25519/Ed(wards)25519 that don't (and RSA)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented it, so please check it out.
|
Okay wow, most of the lines are Cargo.lock changes, it's actually not that bad! Sorry I took so long to review it. |
|
@tarcieri Thank you. You're right, I should have mentioned up front that most of the updates are to Cargo.lock. We've responded to your reviews. Are you nearing the finish line? |
tarcieri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of getting this repo updated I'll go ahead and merge this, but it would be good to at least validate this still builds on no_std targets
|
@nabetti1720 Is there any trick that manages complicated Cargo.lock like this? |
This PR updates the crate to the Release Candidate version currently in development by the RustCrypto project.
ed25519_dalek::SigningKey::from_sec1_der()no longer exists,We replaced it with my own implementation. It may be better to simply not support it.It now returns a message that it is not supported.rand_core::OsRnghas been moved to therandcrate.We felt that there should be no new dependency on theInstead,randcrate, and instead have a small implementation withCryptoRng + RngCoretrait functionality for compatibility.getrandom::SysRngis used.If this has already been planned by an official expert, you may close this PR.
I hope this PR will be helpful to you. :)